🏘️ feat: Add Hosted App Control Plane - #58
Conversation
2d286a4 to
e49f277
Compare
danny-avila
changed the title
00c6ce7 to
fad7fff
Compare
|
@codex review Please review the current PR head fad7fff. Confirm that this exact commit is the reviewed commit and ignore findings that apply only to earlier heads. The stack was rebased onto merged #57/current main; |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fad7fffc90
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const checkpointKey = exactRevision && prior?.hosted_app?.checkpoint_key | ||
| ? prior.hosted_app.checkpoint_key | ||
| : await this.deps.captureCheckpoint(input.sourceRuntimeSessionId, input, signal); |
There was a problem hiding this comment.
When the source workspace is checkpointed again after launching an app, checkpointSession() calls CheckpointStore.pruneOlderThan() and deletes the older object referenced here. If that hosted VM later expires or crashes, exact-revision recovery reuses the now-missing key, restoreSession() returns fetch_failed, and the immutable revision cannot be relaunched without changing its revision. Hosted snapshots need independent retention or pruning awareness.
Useful? React with 👍 / 👎.
| pauseAndDrain(pyWorker, 'Python'), | ||
| pauseAndDrain(otherWorker, 'Other') | ||
| pauseAndDrain(otherWorker, 'Other'), | ||
| ...(hostedAppWorker ? [pauseAndDrain(hostedAppWorker, 'Hosted app')] : []), |
There was a problem hiding this comment.
During a worker rollout, pause(false) waits for an active hosted job, but the enclosing gracefulShutdown() still forcibly exits after 30 seconds even though hosted starts are budgeted for several multi-minute checkpoint and launch legs. A normal long start can therefore be killed mid-transition, after which the stalled retry encounters the per-app Redis lock until its roughly twelve-minute default TTL expires. The shutdown deadline should accommodate these jobs or explicitly abort them with cleanup rather than claiming to drain them.
Useful? React with 👍 / 👎.
| const safeLocation = location | ||
| ? rewriteHostedAppLocation(location, upstream.toString()) | ||
| : undefined; |
There was a problem hiding this comment.
When an app framework honors the forwarded host and emits an absolute Location for its browser-facing hosted-app origin, this check compares it only with the private AWS endpoint origin and converts the otherwise same-origin redirect into a 502. Since the proxy deliberately supplies X-Forwarded-Host, it should also recognize that exact public origin and rewrite it to a relative location while continuing to reject other origins.
Useful? React with 👍 / 👎.
| const retryToken = `${clientToken}-r1`; | ||
| const vm = await this.launchOnce(retryToken, deadlineAtMs, signal, deadlineSignal) | ||
| .catch(second => { throw launchFailure(second); }); |
There was a problem hiding this comment.
If the first VM reaches a terminal state and the -r1 attempt returns a PENDING VM near the shared launch deadline, the deadline can abort this attempt while it is polling. The raw sleep abort is passed through launchFailure() as a non-transient error, so the control plane retires the persisted original-token intent even though the -r1 VM may be live; its ID and retry token never reach the caller, and the next request can launch another VM. Timeout or caller-abort errors from this second attempt must preserve a replayable intent rather than being classified as definite failure.
Useful? React with 👍 / 👎.
| const response = await this.deps.fetch( | ||
| `${normalizeHostedAppMicrovmEndpoint(vm.endpoint ?? '')}/api/v2/hosted-app/start`, | ||
| { | ||
| method: 'POST', | ||
| headers: { | ||
| [token.headerName]: token.token, | ||
| ...microvmPortHeaders(this.config.controlPort), | ||
| 'X-Runtime-Session-Id': runtimeSessionId, | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| body: JSON.stringify(spec), | ||
| signal: AbortSignal.any([ | ||
| callerSignal, | ||
| AbortSignal.timeout(this.config.appStartTimeoutMs), | ||
| ]), | ||
| }, | ||
| ); |
There was a problem hiding this comment.
When reasserting an existing RUNNING revision, a network reset or the resident-start timeout rejects this fetch() with a raw transport error. The fast recovery path only recycles the VM for transient HostedAppMicrovmError instances, so the raw error leaves the stale VM recorded as RUNNING and repeated starts do not enter the intended terminate-and-restore path. Non-caller transport failures here should be converted to a transient hosted-app error.
Useful? React with 👍 / 👎.
| res.setHeader('Set-Cookie', [ | ||
| `${COOKIE_NAME}=${encodeURIComponent(sessionToken)}`, | ||
| 'Path=/', | ||
| 'HttpOnly', | ||
| 'Secure', | ||
| 'SameSite=Strict', | ||
| `Max-Age=${maxAge}`, |
There was a problem hiding this comment.
Outside production, startup explicitly permits an http: preview origin, but this authorization response always sets a Secure __Host- cookie. Browsers ignore that cookie on ordinary insecure origins, so the 303 reaches / without the session token and immediately returns Preview authorization required. Either require HTTPS in every environment or use a development cookie policy compatible with the allowed HTTP origin.
Useful? React with 👍 / 👎.
| && record.hard_deadline_at <= now | ||
| ) return 'stopped'; | ||
| if (record.state === 'RUNNING') return 'running'; | ||
| if (record.state === 'PENDING') return 'starting'; |
There was a problem hiding this comment.
An ambiguous provider failure deliberately leaves a no-ID record in PENDING, but once its provider lifetime and recovery window have elapsed there can no longer be a launch in progress. This unconditional mapping continues to return starting for the remaining Redis-record lifetime, misleading clients that poll status instead of issuing another start. Expired PENDING intents should transition to a terminal public state based on their deadline.
Useful? React with 👍 / 👎.
| if ( | ||
| exactRevision | ||
| && prior?.state === 'RUNNING' | ||
| && prior.microvm_id | ||
| && prior.endpoint | ||
| && (prior.hard_deadline_at == null | ||
| || prior.hard_deadline_at > this.now() + HOSTED_APP_DEADLINE_HEADROOM_MS) |
There was a problem hiding this comment.
The RUNNING fast path checks only the app revision/spec and ignores the persisted launch_fingerprint. After a deployment changes the hosted image version, execution role, ingress connectors, idle policy, or lifetime limits, starting the same revision therefore reasserts the old VM for up to its existing eight-hour lease instead of replacing it under the current launch and security policy. Require the current launch fingerprint before reusing a running VM.
Useful? React with 👍 / 👎.
| if (!hostedAppPreviewCredentialUsable(record, resolved, Date.now(), PREVIEW_REFRESH_SKEW_MS)) { | ||
| await submitHostedAppJob('hosted-app:refresh-preview', { | ||
| operation: 'refresh-preview', | ||
| hostedAppRuntimeId: resolved.hostedAppRuntimeId, | ||
| tenantId: record.tenant_id, | ||
| canonicalUserId: record.canonical_user_id, | ||
| _otel: captureTraceCarrier(), | ||
| }, `happ-refresh-${resolved.hostedAppRuntimeId}-${Math.floor(Date.now() / 30_000)}`); |
There was a problem hiding this comment.
During the final 60 seconds of an otherwise valid AWS credential, every preview request synchronously awaits this refresh job. If Redis, the hosted worker, or the per-app lease is temporarily unavailable, the exception escapes immediately and the gateway rejects the request even though the existing credential has not expired; the subsequent minimum-zero usability check is never reached. After a failed refresh, reread state and use the still-valid credential before returning an outage.
Useful? React with 👍 / 👎.
| bytes > env.MAX_FILE_SIZE | ||
| ? new HostedAppControlPlaneError( | ||
| 'hosted_app_request_too_large', | ||
| `Hosted app request exceeds ${env.MAX_FILE_SIZE} bytes`, | ||
| 413, |
There was a problem hiding this comment.
For a request without Content-Length, exceeding the limit raises this typed 413 from inside the body stream, but Node's fetch() rejects with a top-level transport TypeError whose cause is the stream error. The gateway consequently treats the request as an unknown upstream failure and returns 502 instead of the intended hosted_app_request_too_large response. Unwrap or otherwise propagate the limiter error across the fetch boundary.
Useful? React with 👍 / 👎.
2 participants
Summary
Adds the stateful-profile control-plane half of resident hosted apps on top of #57.
Safety and lifecycle properties
(app_id, revision, spec)contractTERMINATINGbefore the AWS call, so later Redis/generation failures cannot leave a dead VM advertised as runningstartedAtanchors the lease deadline; ambiguous expiry also budgets provider-call latencyBrowser boundary
A top-level app document can still navigate the owner's browser to another origin. This experimental viewer is therefore owner-trusted. Before broad untrusted enablement, app content must be placed on a separate origin inside a sandboxed gateway wrapper. The runbook states this restriction explicitly.
Verification
bun test service/src/hosted-app/*.test.ts service/src/middleware/httpMetrics.test.ts service/src/secure-startup.test.ts— 82 passbun run testinservice— 604 passbun run buildinservice— pass (only two pre-existing TS2352 warnings)git diff --check— pass00c6ce7Dependency
Stacked on #57 at
7aaab6c. Review this PR as the service/control-plane continuation; retarget it tomainafter #57 lands.